Is it really necessary to have these timeouts here and is there nothing in electrum client? It surprises me because apparently retries are implemented in the client code, but not timeouts?

There is a timeout parameter but we previously learned on the Esplora side to not necessarily trust the timeout parameters of the libraries we use (namely, reqwest which is used by esplora-client showed delayed handling of concurrent requests if the futures were polled from different threads). So we introduce reliable timeouts on top of the library-provided ones as a belt-and-suspender measure to make sure we never get into a state where a misbehaving task can lock up all of our worker threads. I guess we could consider to enable the timeouts on electrum-client in addition to the ones we set, but I'm not sure if coming up with yet another magic number is worth it?

Isn't electrum different because it isn't an async library?

I think locking up worker threads can still happen because the async task may finish on the timeout, but the worker thread remains blocked? It isn't cancelled.

Okay. Now added a fixup also setting the timeout parameter on the electrum_client side (where we can).

Is there a point though in having an async timeout wrapper if the spawned thread would hang anyway?

I think so. For one, now setting the timeouts hopefully has the spawned thread returning eventually. But if it would do so for some reason, it would be preferable to at least try to continue (while risking eventually running out of blocking worker threads if the issue persists), instead of just locking up and not continuing with subsequent sync attempts.

I see you also set retries now. I saw that it was 1 previously (the default). Did the earlier test with the wifi disconnect pass because that one retry happened after you already reconnected wifi?

If I understand this correctly, setting the timeout on the electrum client itself is essential to not end up with a potential graveyard of stuck threads? This might be important to add as a comment.

Also curious to hear why you think that retrying over and over again when a previous request is still stuck is a good idea? It will eventually exhaust the thread pool, right? And then stop independent processes within LDK too?

The original reason for the double timeout is because esplora-client turned out to be not trustworthy. But, as suggested above, isn't electrum different because it isn't an async library and it is unlikely for the timeout to fail? Looking at the sources, it seems that the timeout is set on a pretty low level for all calls.

I see you also set retries now. I saw that it was 1 previously (the default). Did the earlier test with the wifi disconnect pass because that one retry happened after you already reconnected wifi?

No, I think the calls were cancelled and we then simply continued on the next timer tick.

If I understand this correctly, setting the timeout on the electrum client itself is essential to not end up with a potential graveyard of stuck threads? This might be important to add as a comment.

I don't think it's super essential as in most circumstances when the connection would fail, we'd still error once the connection is dropped, IIUC. But yes, we probably don't want to lean too much on behavior that might also very well OS-specific.

Also curious to hear why you think that retrying over and over again when a previous request is still stuck is a good idea? It will eventually exhaust the thread pool, right? And then stop independent processes within LDK too?

Chain syncing and fee rate updates are critical for the operation of a Lightning node. When some process would really get stuck for an unexpected reason, we basically have two options: panic, hoping that a restart or operator intervention would fix the issue, or retry, risking that we might exhaust the thread pool eventually if the exact same unexpected behavior persists 512 times in a row. I think the latter is preferable.

The original reason for the double timeout is because esplora-client turned out to be not trustworthy. But, as suggested above, isn't electrum different because it isn't an async library and it is unlikely for the timeout to fail? Looking at the sources, it seems that the timeout is set on a pretty low level for all calls.

Yeah, tbh. I mostly added it for the sake of uniformity across the different chain sources and as a general 'belt-and-suspenders' kind of thing. If we don't think 'belt-and-suspenders' are needed, we could consider dropping the timeouts everywhere and relying on the dependencies. I'm on the fence about going this way.

But, tbh., I currently don't fully see why being doubly cautious in this amalgamation between async runtimes and blocking threads is a bad thing, especially since the behavior of the dependency crates might very well (silently) change down the line, even if we review their code now and are positive that the double timeouts are not required in one case or another. FWIW, there is a reason why we added them in the first place, namely that users in production had their node lock up for a really long time due to completely unpredictable behavior arising from reusing reqwest clients across different thread contesxts (reusing clients is explicitly endorsed in their docs, btw).

I see you also set retries now. I saw that it was 1 previously (the default). Did the earlier test with the wifi disconnect pass because that one retry happened after you already reconnected wifi?

If I understand this correctly, setting the timeout on the electrum client itself is essential to not end up with a potential graveyard of stuck threads? This might be important to add as a comment.

Also curious to hear why you think that retrying over and over again when a previous request is still stuck is a good idea? It will eventually exhaust the thread pool, right? And then stop independent processes within LDK too?

The original reason for the double timeout is because esplora-client turned out to be not trustworthy. But, as suggested above, isn't electrum different because it isn't an async library and it is unlikely for the timeout to fail? Looking at the sources, it seems that the timeout is set on a pretty low level for all calls.